-
-
Notifications
You must be signed in to change notification settings - Fork 169
fix (cli + hql): adding checks for fly cli name length, replacing underscore, and adding checks for duplicate field names, hql panic removals #722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
xav-db
wants to merge
7
commits into
dev
Choose a base branch
from
fli-cli-checks
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, no comments
<!-- greptile_comment -->
<h2>Greptile Overview</h2>
<h3>Greptile Summary</h3>
Replaces `panic!()` and `unreachable!()` macros throughout the helixc
compiler with proper error handling using descriptive error messages.
## Key Changes
- **Added two new error codes**: E627 for shortest path algorithms
missing required `from`/`to` parameters, and E628 for invalid `DROP`
usage on non-traversals
- **Replaced 4 panics in graph step validation**: ShortestPath,
ShortestPathDijkstras, ShortestPathBFS, and ShortestPathAStar now return
E627 errors instead of panicking when both `from` and `to` are missing
- **Replaced panic in statement validation**: `DROP` statement now
generates E628 error when applied to non-traversal expressions
- **Replaced 2 panics in traversal validation**: Invalid field value
types now generate E206 errors with `GeneratedValue::Unknown` fallback
- **Replaced 15+ `unreachable!()` calls in parser**: All parser methods
now return descriptive `ParserError` messages for unexpected grammar
rules
## Impact
This PR significantly improves compiler robustness by ensuring invalid
HelixQL queries produce helpful error messages instead of crashing the
compiler. All panic paths have been replaced with proper error
propagation that provides context to users about what went wrong.
<details><summary><h3>Important Files Changed</h3></summary>
File Analysis
| Filename | Score | Overview |
|----------|-------|----------|
| helix-db/src/helixc/analyzer/error_codes.rs | 5/5 | Added two new
error codes (E627 for missing shortest path parameters, E628 for invalid
DROP usage) with proper documentation and implementations |
| helix-db/src/helixc/analyzer/methods/graph_step_validation.rs | 5/5 |
Replaced four `panic!()` calls with proper E627 error generation for
shortest path algorithms missing both from/to parameters |
| helix-db/src/helixc/analyzer/methods/statement_validation.rs | 5/5 |
Replaced panic with E628 error when DROP is applied to non-traversal
expressions |
| helix-db/src/helixc/parser/expression_parse_methods.rs | 5/5 |
Replaced three `unreachable!()` macros with descriptive error messages
for unexpected parser rules |
| helix-db/src/helixc/parser/graph_step_parse_methods.rs | 5/5 |
Replaced four `unreachable!()` macros with proper ParserError for
unexpected rules in order_by and shortest path parsing |
</details>
</details>
<details><summary><h3>Sequence Diagram</h3></summary>
```mermaid
sequenceDiagram
participant User
participant Parser
participant Analyzer
participant ErrorHandler
Note over Parser,Analyzer: Before: panics on unexpected input
User->>Parser: Submit HelixQL query
alt Unexpected parse rule encountered
Parser->>Parser: Match rule
Parser-->>ErrorHandler: Return ParserError with context
ErrorHandler-->>User: Descriptive error message
end
alt Valid parse, needs analysis
Parser->>Analyzer: Pass parsed AST
alt ShortestPath missing from/to
Analyzer->>Analyzer: Validate graph step
Analyzer-->>ErrorHandler: generate_error!(E627)
ErrorHandler-->>User: "requires either `from` or `to` parameter"
end
alt DROP on non-traversal
Analyzer->>Analyzer: Validate DROP statement
Analyzer-->>ErrorHandler: generate_error!(E628)
ErrorHandler-->>User: "DROP can only be applied to traversals"
end
alt Invalid field value type
Analyzer->>Analyzer: Validate field values
Analyzer-->>ErrorHandler: generate_error!(E206)
ErrorHandler-->>User: "invalid value type"
end
end
Note over Parser,ErrorHandler: After: graceful error handling
```
</details>
<!-- greptile_other_comments_section -->
<!-- /greptile_comment -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Greptile Overview
Greptile Summary
Added validation safeguards to prevent Fly.io deployment failures and schema definition errors.
Key Changes:
fly.tomlfiles before deployment to prevent conflictsImportant Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant User participant FlyManager participant FlyAPI participant SchemaAnalyzer Note over User,SchemaAnalyzer: Fly.io Deployment Flow User->>FlyManager: init_app(instance_name, config) FlyManager->>FlyManager: app_name() - sanitize underscores to hyphens FlyManager->>FlyManager: Check app_name.len() <= 32 alt Name too long FlyManager-->>User: Error: exceeds MAX_APP_NAME_LENGTH end FlyManager->>FlyManager: read_app_name_from_fly_toml() alt fly.toml exists FlyManager->>FlyAPI: app_exists(existing_app_name) FlyAPI-->>FlyManager: true/false alt App exists FlyManager-->>User: Error: fly.toml app already exists end end FlyManager->>FlyAPI: app_exists(app_name) FlyAPI-->>FlyManager: true/false alt App exists FlyManager-->>User: Error: target app already exists end FlyManager->>FlyAPI: Create app FlyAPI-->>User: Success Note over User,SchemaAnalyzer: Schema Validation Flow User->>SchemaAnalyzer: Define schema with fields SchemaAnalyzer->>SchemaAnalyzer: Initialize seen_fields HashSet loop For each field SchemaAnalyzer->>SchemaAnalyzer: lowercase field name SchemaAnalyzer->>SchemaAnalyzer: seen_fields.insert(lower_name) alt Duplicate detected SchemaAnalyzer->>SchemaAnalyzer: push_schema_err(E109) end SchemaAnalyzer->>SchemaAnalyzer: Check reserved names SchemaAnalyzer->>SchemaAnalyzer: Check field type validity end SchemaAnalyzer-->>User: Validation complete with diagnostics